Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL] Fix device code instrumentation #4615

Merged
merged 14 commits into from
Sep 29, 2021

Conversation

AGindinson
Copy link
Contributor

@AGindinson AGindinson commented Sep 22, 2021

  • Fix the malfunctioning driver option for passing -fsycl-instrument-device-code to CC1
    for SPIR-V-based targets.
  • Add the ITT device libraries when the CLI option is enabled, excluding these from
    -fno-sycl-device-lib effects.
  • Disable ITT annotations for ESIMD code.

Tested in intel/llvm-test-suite#484.

Signed-off-by: Artem Gindinson artem.gindinson@intel.com

MrSidims
MrSidims previously approved these changes Sep 22, 2021
MrSidims
MrSidims previously approved these changes Sep 22, 2021
@AGindinson
Copy link
Contributor Author

Moving to draft - it is possible that defaulting the FE option to true and removing the malfunctioning driver option entirely should be done immediately.

@AGindinson AGindinson marked this pull request as draft September 22, 2021 10:53
Instead of maintaining a (malfunctioning) driver option,
unconditionally pass the CC1 option `-fsycl-instrument-device-code`
for SPIR-V-based targets.

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
@AGindinson AGindinson changed the title [SYCL][Driver] Fix device code instrumentation [SYCL] Unconditionally enable SPIR ITT annotations Sep 22, 2021
@AGindinson AGindinson marked this pull request as ready for review September 22, 2021 16:25
MrSidims
MrSidims previously approved these changes Sep 22, 2021
premanandrao
premanandrao previously approved these changes Sep 22, 2021
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me.

mdtoguchi
mdtoguchi previously approved these changes Sep 22, 2021
Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
The addition of internal libraries cannot be affected by `-fno-sycl-device-lib`

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
@AGindinson
Copy link
Contributor Author

AGindinson commented Sep 27, 2021

Still TBD:

  • Add an E2E test with device code instrumentation ON
  • Address Konst's comment

REMOVED: "Rework the parsing of the CLI option so that the non-SPIR-triple error is emitted both for compile and link phases"
(not needed for now since device libraries are only used for SPIR-V based targets)

@AGindinson AGindinson changed the title [SYCL] Unconditionally enable SPIR ITT annotations [SYCL] Fix device code instrumentation Sep 27, 2021
Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
Instead of skipping the annotations during ESIMD-specific module
processing, don't add the instrumentation to these GPU-specific
kernels in the first place.

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
AGindinson added a commit to AGindinson/llvm-test-suite that referenced this pull request Sep 28, 2021
Add a couple tests with execute-only kernels to ensure that ITT
annotations' generation doesn't break the compilation/execution.

This aims to test intel/llvm#4615.

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
@AGindinson
Copy link
Contributor Author

@premanandrao @elizabethandrews @AaronBallman Just FYI, there are no changes expected to Clang FE with this revision of the PR, so feel free to unsubscribe if the notifications are bugging you.

@premanandrao
Copy link
Contributor

@premanandrao @elizabethandrews @AaronBallman Just FYI, there are no changes expected to Clang FE with this revision of the PR, so feel free to unsubscribe if the notifications are bugging you.

Thanks for the note, but no worries. :-)

Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>
Copy link
Contributor

@vzakhari vzakhari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPIRITTAnnotationsPass changes LGTM.

Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AGindinson
Copy link
Contributor Author

The Precommit checks have passed after being tuned to the latest revision of intel/llvm-test-suite#484, approvals for major changes have been gathered. @kbobrovs, could you please review the changes in sycl/test/esimd/vadd.cpp RUN lines and the comment resolution?

@AlexeySachkov AlexeySachkov merged commit 5667aba into intel:sycl Sep 29, 2021
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Oct 3, 2021
* sycl: (108 commits)
  [SYCL][XPTI] Revisit resource management strategy (intel#4494)
  [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681)
  [SYCL] Make kernel_bundle interop more conformant (intel#4672)
  [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667)
  [SYCL] Add support for std::byte to vec class  (intel#4637)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671)
  [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668)
  [SYCL] Exclude exported symbols from kernel bundles (intel#4660)
  Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659)
  [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639)
  [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657)
  [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629)
  [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663)
  [SYCL] Fix device code instrumentation (intel#4615)
  Remove myself as a code owner (intel#4653)
  [SYCL] Fix overwriting insert to sub_group_mask (intel#4656)
  [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641)
  [SYCL][Doc] Add device global extension spec (intel#4233)
  [sycl-post-link] Adds property listing exported functions (intel#4626)
  [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644)
  ...
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Oct 3, 2021
* sycl: (107 commits)
  [SYCL][XPTI] Revisit resource management strategy (intel#4494)
  [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681)
  [SYCL] Make kernel_bundle interop more conformant (intel#4672)
  [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667)
  [SYCL] Add support for std::byte to vec class  (intel#4637)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671)
  [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668)
  [SYCL] Exclude exported symbols from kernel bundles (intel#4660)
  Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659)
  [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639)
  [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657)
  [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629)
  [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663)
  [SYCL] Fix device code instrumentation (intel#4615)
  Remove myself as a code owner (intel#4653)
  [SYCL] Fix overwriting insert to sub_group_mask (intel#4656)
  [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641)
  [SYCL][Doc] Add device global extension spec (intel#4233)
  [sycl-post-link] Adds property listing exported functions (intel#4626)
  [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644)
  ...
@AGindinson AGindinson deleted the instr-device-code branch January 1, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants